core/: Introduce `rsa` feature flag to avoid `ring` dependency by GamePad64 · Pull Request #2860 · libp2p/rust-libp2p

GamePad64 · 2022-08-31T10:45:45Z

Description

I've tried to build libp2p for router, running mips cpu, but the build has failed because of ring dependency. Ring is used mainly for RSA in libp2p-code, so I've made rsa as an optional feature

Links to any relevant issues

#1396
#1975

Open Questions

Change checklist

I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
A changelog entry has been made in the appropriate crates

thomaseizinger

Generally, I am happy to land this change. Please see inline comments :)

core/CHANGELOG.md

thomaseizinger

Thank you for doing all the version updates and changelog entries!

One more question and I'd like to have @mxinden's review on this as well before we merge it.

core/src/identity/error.rs

thomaseizinger · 2022-09-01T16:09:53Z

I've tried to build libp2p for router, running mips cpu, but the build has failed because of ring dependency. Ring is used mainly for RSA in libp2p-code, so I've made rsa as an optional feature

One more note on this, I am pretty sure libp2p-quic - once merged - will also bring in a dependency on ring again. Using QUIC is obviously optional but I just wanted to mention that RSA keys are not the only thing that ties us to ring.

GamePad64 · 2022-09-01T23:04:20Z

libp2p-core is a central part of libp2p, so removing this problematic dependency from it would be much better. libp2p-quic is not a required transport, so we can just continue to use libp2p-tcp 😉
The biggest problem with ring, that it supports only x86 and arm in 32- and 64-bit variants. Using it denies building your apps for embedded targets, supported in Rust (like routers and such).

…ptional-ring

thomaseizinger

Thanks!

One more note regarding using the new dep syntax.

core/Cargo.toml

thomaseizinger · 2022-09-04T09:11:53Z

transports/noise/src/protocol/x25519.rs

        Ok(PublicKey(X25519(pk)))
    }

+    #[allow(irrefutable_let_patterns)]


This is unfortunate :/

Yes, but I don't know how to change this without a massive rewrite :(

thomaseizinger · 2022-09-04T09:29:46Z

Thanks for adopting the suggestions so quickly. Just waiting for @mxinden now :)

mxinden

Thanks for the patch and thanks for following this all the way through, updating the many versions 🙏.

This looks good to me apart from one thing.

The libp2p-core feature secp256k1 is passed through to libp2p. In addition the secp256k1 is part of the libp2p default feature.

According to the libp2p specification:

Implementations MUST support Ed25519. Implementations SHOULD support RSA if they wish to interoperate with the mainline IPFS DHT and the default IPFS bootstrap nodes. Implementations MAY support Secp256k1 and ECDSA, but nodes using those keys may not be able to connect to all other nodes.

https://github.com/libp2p/specs/blob/master/peer-ids/peer-ids.md#key-types

I read this as rsa taking a higher priority as secp256k1. Thus I suggest passing the rsa feature through libp2p and adding rsa to the default feature on libp2p. That would as well remove the breaking change, i.e. the removal of default support for decoding rsa keys.

@GamePad64 @thomaseizinger does the above reasoning make sense?

…ptional-ring

GamePad64 · 2022-09-05T06:53:40Z

Yes, this makes sense to me :) Added rsa feature to base libp2p crate and added it to default.

thomaseizinger

@mxinden That sounds good to me.

It is a breaking change regardless though. A user depending on the crate with disabled default features will not get RSA support whereas previously, they did.

mxinden

Thanks for the follow-ups!

mxinden · 2022-09-07T05:07:35Z

It is a breaking change regardless though. A user depending on the crate with disabled default features will not get RSA support whereas previously, they did.

👍 I will point this out in the release notes on #2869.

mxinden · 2022-09-07T06:15:09Z

🎉 first pull request running the interop tests #2835 //CC @laurentsenta.

…p#2860) - Introduce `rsa` feature flag to `libp2p-core`. - Expose `rsa` feature in `libp2p`. - Add `rsa` feature to `libp2p` `default`.

GamePad64 added 2 commits August 31, 2022 13:31

Allow disabling rsa identity, so ring could be disabled

5a7dec2

update changelog

14d8066

GamePad64 marked this pull request as ready for review August 31, 2022 11:15

thomaseizinger reviewed Aug 31, 2022

View reviewed changes

core/CHANGELOG.md Outdated Show resolved Hide resolved

Bump libp2p-core version, fix warnings in tests

4a2a1a0

thomaseizinger approved these changes Sep 1, 2022

View reviewed changes

core/src/identity/error.rs Outdated Show resolved Hide resolved

Merge branch 'master' of github.com:libp2p/rust-libp2p into feature/o…

202181b

…ptional-ring

thomaseizinger changed the title ~~Allow disabling rsa identity, so ring could be disabled~~ core: Introduce rsa feature flag to avoid ring dependency Sep 2, 2022

thomaseizinger requested a review from mxinden September 2, 2022 08:30

GamePad64 added 2 commits September 2, 2022 16:12

Replace #[allow(dead_code)] with conditional compilation

553b4dd

Merge branch 'master' into feature/optional-ring

8d35f51

thomaseizinger approved these changes Sep 4, 2022

View reviewed changes

Use dep:ring + remove "rsa" feature from defaults

78402c1

Merge branch 'master' into feature/optional-ring

da951ad

mxinden changed the title ~~core: Introduce rsa feature flag to avoid ring dependency~~ core/: Introduce rsa feature flag to avoid ring dependency Sep 5, 2022

mxinden reviewed Sep 5, 2022

View reviewed changes

mxinden mentioned this pull request Sep 5, 2022

*: Prepare v0.48.0 #2869

Merged

5 tasks

GamePad64 added 2 commits September 5, 2022 09:50

Add rsa as a default feature for libp2p

536a068

Merge branch 'master' of github.com:libp2p/rust-libp2p into feature/o…

ac2bca4

…ptional-ring

GamePad64 requested a review from mxinden September 5, 2022 07:12

thomaseizinger approved these changes Sep 5, 2022

View reviewed changes

Merge branch 'master' into feature/optional-ring

6584b4d

mxinden approved these changes Sep 7, 2022

View reviewed changes

mxinden merged commit 8644c65 into libp2p:master Sep 7, 2022

mxinden mentioned this pull request Sep 25, 2022

ping/: Update to rust-libp2p release v0.48.0 and master v0.49.0 libp2p/test-plans#41

Merged

dariusc93 mentioned this pull request Oct 17, 2022

libp2p 0.48 ipfs-rust/ipfs-embed#111

Merged

Conversation

GamePad64 commented Aug 31, 2022

Description

Links to any relevant issues

Open Questions

Change checklist

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thomaseizinger commented Sep 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GamePad64 commented Sep 1, 2022

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

thomaseizinger Sep 4, 2022

Choose a reason for hiding this comment

Uh oh!

GamePad64 Sep 4, 2022

Choose a reason for hiding this comment

Uh oh!

thomaseizinger commented Sep 4, 2022

Uh oh!

mxinden left a comment

Choose a reason for hiding this comment

Uh oh!

GamePad64 commented Sep 5, 2022

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

mxinden left a comment

Choose a reason for hiding this comment

Uh oh!

mxinden commented Sep 7, 2022

Uh oh!

mxinden commented Sep 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

thomaseizinger commented Sep 1, 2022 •

edited

Loading